Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add "StartBlock::Continue" to registry contract/types #548

Merged

Conversation

morgsmccauley
Copy link
Collaborator

@morgsmccauley morgsmccauley commented Feb 5, 2024

The start block parameter (start_block_height) has two states, signifying "Start from block" and "Start from latest" only. With the removal of the continuous "real-time" process, which essentially runs Indexers "From Interruption", the need for a third state arises, representing "Interruption" arises. This PR expands the registry contract/types to accommodate this change. This work was a good opportunity to refactor the existing types, removing unnecessary fields and tailoring it to the new architecture.

Registry Types Refactoring

  • filter: IndexerRule -> rule: Rule: IndexerRule contained a lot of noise/data which was not needed. id, name, and indexer_rule_kind have all been removed. The only useful part here is MatchingRule which has been renamed to just Rule.
  • schema is now non-optional: This field is always required so it makes sense to convey that in the code. Having it as an Option created lots of unnecessary checks throughout the system.
  • start_block_height replaced by start_block: The latter being an enum to represent "From Latest", "From Block", and "From Interruption"

Public methods/API

To minimise the disruption of the existing contract consumers, the public API remains unchanged, i.e. the core methods (list_indexer_functions, register_indexer_function, etc.) use/return the same types. As the underlying data will be migrated to the new types, these methods infer the old types from the new ones. New methods have been created to work with the new types (register, list_all, etc.) allowing clients to move over when possible, as opposed to creating a breaking change.

@morgsmccauley morgsmccauley linked an issue Feb 5, 2024 that may be closed by this pull request
@morgsmccauley morgsmccauley changed the title 536 expand start block types to include from interruption feat: Add "From Interruption" to registry contract/types Feb 5, 2024
@@ -294,7 +294,7 @@ mod tests {
)
.unwrap(),
function_name: "test".to_string(),
indexer_rule: registry_types::IndexerRule {
indexer_rule: registry_types::OldIndexerRule {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be migrated to new types in a future PR

@@ -274,7 +274,7 @@ mod tests {
use mockall::predicate;
use std::collections::HashMap;

use registry_types::{IndexerRule, IndexerRuleKind, MatchingRule, Status};
use registry_types::{IndexerRuleKind, MatchingRule, OldIndexerRule as IndexerRule, Status};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be migrated to new types in a future PR

@morgsmccauley morgsmccauley marked this pull request as ready for review February 5, 2024 08:18
@morgsmccauley morgsmccauley requested a review from a team as a code owner February 5, 2024 08:18
Copy link
Collaborator

@darunrs darunrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we're going forward with Update, or some version of that instead of FromInterruption?

Otherwise it looks good to me!

Seems like the approach was to create new functions to avoid having to build in backwards compatibility and bloat existing code further, migrate our services over to new functions, and then return and clean up the old code?

Comment on lines +45 to +46
RegistryV3,
AccountV3(CryptoHash),
Copy link
Collaborator

@darunrs darunrs Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of these? I'm a bit confused how these are used in conjunction with various types like IndexersByAccount. Seems we use all of them in some way or another.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't completely understand, but afaik, we need to use a unique storage key for each collection we store. Re-using an existing key could lead us to read stale state and therefore fail deserialization. This is essentially a helper for creating unique keys.

In saying that, the migration actually ignores existing state so it's probably ok to ignore these, but I kept doing it just incase 😅

@morgsmccauley
Copy link
Collaborator Author

morgsmccauley commented Feb 7, 2024

I assume we're going forward with Update, or some version of that instead of FromInterruption?

Yup, I'll rename to Continue

Seems like the approach was to create new functions to avoid having to build in backwards compatibility and bloat existing code further, migrate our services over to new functions, and then return and clean up the old code?

Yes exactly, V2 will use the new functions, while V1 still uses the existing. I'll then remove the existing functions once V2 is live.

@morgsmccauley morgsmccauley force-pushed the 536-expand-start-block-types-to-include-from-interruption branch from 642d7f1 to 9e92f71 Compare February 7, 2024 01:34
@morgsmccauley morgsmccauley changed the title feat: Add "From Interruption" to registry contract/types feat: Add "Continue" to registry contract/types Feb 7, 2024
@morgsmccauley morgsmccauley changed the title feat: Add "Continue" to registry contract/types feat: Add "StartBlock::Continue" to registry contract/types Feb 7, 2024
@morgsmccauley morgsmccauley linked an issue Feb 7, 2024 that may be closed by this pull request
@morgsmccauley morgsmccauley force-pushed the 536-expand-start-block-types-to-include-from-interruption branch from 11d61f6 to cc7ad3e Compare February 7, 2024 02:14
@morgsmccauley
Copy link
Collaborator Author

This has been deployed to dev, will let it sit for a bit and then to prod

@morgsmccauley morgsmccauley merged commit c92f477 into main Feb 7, 2024
8 checks passed
@morgsmccauley morgsmccauley deleted the 536-expand-start-block-types-to-include-from-interruption branch February 7, 2024 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the contract to allow only admins to use this type Expand start block types to include "Continue"
2 participants